Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Autoload the parsers on first use #66

Merged
merged 1 commit into from
May 14, 2024
Merged

Autoload the parsers on first use #66

merged 1 commit into from
May 14, 2024

Conversation

jrafanie
Copy link
Contributor

Adding this gem to an application should probably load the gettext parsing utilities when it's being used at runtime. Currently, the gem entry lib file requires parser which eager loads these parsers and consumes time and memory when the developer or system may not need them.

before this PR

% for i in $(seq 1 10); do; ruby in_line_test.rb |grep MB; done
1.828125 MB
1.9375 MB
1.796875 MB
1.546875 MB
1.890625 MB
1.546875 MB
1.8125 MB
2.40625 MB
1.9375 MB
1.6875 MB

after this PR

% for i in $(seq 1 10); do; ruby in_line_test.rb |grep MB; done
0.90625 MB
0.53125 MB
0.984375 MB
1.1875 MB
0.46875 MB
0.390625 MB
0.453125 MB
0.96875 MB
0.546875 MB
0.5 MB

Using this script:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "rails"
  # If you want to test against edge Rails replace the previous line with this:
  # gem "rails", github: "rails/rails", branch: "main"

  gem 'gettext_i18n_rails'
  gem 'gettext', '>=3.0.2', :require => false
  # gem 'ruby_parser', :require => false, :group => :development
  # gem "gettext_i18n_rails_js", "~> 2.0", :require => false, :git => "https://github.com/jrafanie/gettext_i18n_rails_js.git", :branch => "autoload_parsers_on_first_use"
  gem "gettext_i18n_rails_js", "~> 2.0", :require => false
  gem 'derailed_benchmarks', group: :development
end

require "active_support"
require "active_support/core_ext/object/blank"
require "minitest/autorun"

require 'get_process_mem'
mem    = GetProcessMem.new
before = mem.mb

require 'gettext_i18n_rails_js'

after  = mem.mb
puts "#{after - before} MB"

@jrafanie
Autoload the parsers on first use
b32fae8 
Adding this gem to an application should probably load the gettext
parsing utilities when it's being used at runtime.  Currently, the
gem entry lib file requires parser which eager loads these parsers
and consumes time and memory when the developer or system may not
need them.
tboerger
tboerger approved these changes May 11, 2024
View reviewed changes
Copy link
Member

@tboerger tboerger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great contribution!

@tboerger
Copy link
Member

Fails for ruby 2.7, I'm thinking about dropping all ruby versions which are already eol, like everything below 3.1.

@tboerger
Copy link
Member

Or at least below 3.0

@jrafanie
Copy link
Contributor Author

@tboerger It looks like #65 is also failing on 2.7 with rails 7 for the same io console reason:

bundle exec rspec
  shell: /usr/bin/bash -e {0}
Gem::LoadError: You have already activated io-console 0.5.6, but your Gemfile requires io-console 0.7.2. Since io-console is a default gem, you can either remove your dependency on it or try updating to a newer version of bundler that supports io-console as a default gem.
  /opt/hostedtoolcache/Ruby/2.7.8/x64/lib/ruby/gems/2.7.0/gems/bundler-1.17.[3](https://github.com/webhippie/gettext_i18n_rails_js/actions/runs/9038144306/job/24850487097?pr=66#step:4:3)/lib/bundler/runtime.rb:319:in `check_for_activated_spec!'
bundler: failed to load command: rspec (/home/runner/work/gettext_i18n_rails_js/gettext_i18n_rails_js/vendor/bundle/ruby/2.7.0/bin/rspec)
  /opt/hostedtoolcache/Ruby/2.7.8/x6[4](https://github.com/webhippie/gettext_i18n_rails_js/actions/runs/9038144306/job/24850487097?pr=66#step:4:5)/lib/ruby/gems/2.7.0/gems/bundler-1.17.3/lib/bundler/runtime.rb:31:in `block in setup'
  /home/runner/work/gettext_i18n_rails_js/gettext_i18n_rails_js/vendor/bundle/ruby/2.7.0/gems/forwardable-1.3.3/lib/forwardable.rb:240:in `each'
  /home/runner/work/gettext_i18n_rails_js/gettext_i18n_rails_js/vendor/bundle/ruby/2.7.0/gems/forwardable-1.3.3/lib/forwardable.rb:240:in `each'
  /opt/hostedtoolcache/Ruby/2.7.8/x[6](https://github.com/webhippie/gettext_i18n_rails_js/actions/runs/9038144306/job/24850487097?pr=66#step:4:7)4/lib/ruby/gems/2.7.0/gems/bundler-1.17.3/lib/bundler/runtime.rb:26:in `map'
  /opt/hostedtoolcache/Ruby/2.7.8/x64/lib/ruby/gems/2.7.0/gems/bundler-1.1[7](https://github.com/webhippie/gettext_i18n_rails_js/actions/runs/9038144306/job/24850487097?pr=66#step:4:8).3/lib/bundler/runtime.rb:26:in `setup'
  /opt/hostedtoolcache/Ruby/2.7.8/x64/lib/ruby/gems/2.7.0/gems/bundler-1.17.3/lib/bundler.rb:107:in `setup'
  /opt/hostedtoolcache/Ruby/2.7.[8](https://github.com/webhippie/gettext_i18n_rails_js/actions/runs/9038144306/job/24850487097?pr=66#step:4:9)/x64/lib/ruby/gems/2.7.0/gems/bundler-1.17.3/lib/bundler/setup.rb:20:in `<top (required)>'
  /opt/hostedtoolcache/Ruby/2.7.8/x64/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:83:in `require'
  /opt/hostedtoolcache/Ruby/2.7.8/x64/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:83:in `require'

I'd hate to drop 2.7 since it's supported with rails 7.0 unless it's truly required.

I can't recreate it locally with ruby 2.7/bundler 1.17/rails 7.0, so I'm wondering if it's specific to the CI environment.

My first thought is loosening the bundler 1.17 requirement.

Any ideas?

@jrafanie
Copy link
Contributor Author

jrafanie commented May 13, 2024 •
edited
Loading

It works for me locally. I'll close and reopen to try it again but it expect it to fail similarly to #65

% ruby -v
ruby 2.7.8p225 (2023-03-30 revision 1f4d455848) [arm64-darwin22]
% RAILS_VERSION="7.0.0" bundle _1.17.3_ --version
Bundler version 1.17.3
% RAILS_VERSION="7.0.0" bundle _1.17.3_ show rails
/Users/joerafaniello/.gem/ruby/2.7.8/gems/rails-7.0.8.1
% RAILS_VERSION="7.0.0" bundle _1.17.3_ exec rspec
...
% echo $?
0

@jrafanie jrafanie closed this May 13, 2024
@jrafanie jrafanie reopened this May 13, 2024
@jrafanie
Copy link
Contributor Author

Ok, I opened #67 to see if we can get it to pass with ruby 2.7, rails 6.1, and bundler 2.4 as the minimums for testing.

@tboerger tboerger merged commit 8cf59cd into webhippie:master May 14, 2024
36 of 39 checks passed
@jrafanie jrafanie deleted the autoload_parsers_on_first_use branch May 14, 2024 13:08
@tboerger
Copy link
Member

I have published this change as v2.1.0 now, thanks for your effort.

@jrafanie
Copy link
Contributor Author

I have published this change as v2.1.0 now, thanks for your effort.

Thank you! I opened #69 for a minor typo. Have a great day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tboerger tboerger tboerger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jrafanie @tboerger